GH4890: Add typed Cake aliases for dotnet tool subcommands#4846
Conversation
devlead
left a comment
devlead
left a comment
There was a problem hiding this comment.
Please address code compilation issues, first after that this will be reviewed.
paulomorgado
force-pushed
the
feature/implement-dotnet-tool-commands
branch
2 times, most recently
from
22fd64d to
e68c3a7
Compare
OOPS! Fixed! |
devlead
left a comment
devlead
left a comment
There was a problem hiding this comment.
--verbosity is emitted for subcommands that do not support it
Thanks for the thorough coverage here, the alias shape, settings types, and tests all look consistent with the rest of our DotNet tooling.
One concern I'd like addressed before merge: all of the new settings types inherit DotNetSettings.Verbosity, and because these aliases route through DotNetToolRunner, --verbosity is always appended via AppendCommonArguments.
That works for some dotnet tool subcommands, but not all of them.
I verified this against the CLI locally:
- Supported:
install,update,exec,restore - Not supported:
run,list,search,uninstall
For example,dotnet tool run --verbosity minimalfails at runtime because--verbosityis treated as the tool command name.
The unit tests currently assert argument strings that include --verbosity for unsupported commands as well (for example, RunFull, ListFull, SearchFull, and UninstallFull). Those tests pass because they only verify rendered arguments, not actual CLI behavior, so this regression would not be caught in CI.
Impact: If a user sets Verbosity on DotNetToolRunSettings, DotNetToolListSettings, DotNetToolSearchSettings, or DotNetToolUninstallSettings, the resulting command will fail when executed.
Suggestion: Please either:
- Only append
--verbosityfor subcommands that support it, or - Use per-command runners/argument builders (similar to the workload commands),
Minor notes
A few smaller things I noticed:
No integration tests
Other DotNet aliases have coverage under tests/integration/Cake.Common/Tools/DotNet/. The unit tests here are thorough, but a lightweight integration test (even just install + list + uninstall against a manifest) would give extra confidence that the rendered commands work end-to-end.
Different architecture than workload commands
Workload subcommands use dedicated runners per command; this PR centralizes through DotNetTool. That's a reasonable trade-off for less duplication, just noting the pattern difference in case we want consistency long-term.
Temporary mutation of settings.ArgumentCustomization
RunDotNetToolCommandWithForwardedArguments temporarily replaces ArgumentCustomization and restores it in finally. That's tested for DotNetToolExecute but not for DotNetToolRun. Unlikely to matter in practice, but reusing the same settings instance across parallel tasks could theoretically race.
Large file
src/Cake.Common/Tools/DotNet/DotNetAliases.ToolCommands.cs is quite large; it would be better if each sub-command was in its own file.
paulomorgado
force-pushed
the
feature/implement-dotnet-tool-commands
branch
2 times, most recently
from
9bb4b9c to
da92706
Compare
|
|
@paulomorgado integration tests can be tested locally by calling main cake script in repo root like this dotnet cake --target="Run-Integration-Tests"You can test just a specific integration test target with increased verbosity like below dotnet cake --target="Run-Integration-Tests" --integration-tests-target="Cake.Common.Tools.DotNet.DotNetAliases.DotNetToolInstallListUninstallLocal" --integration-tests-verbosity=DiagnosticIf you got en error in the integration test script, then you can execute tests without needed rebuild and execute unit tests by passing the dotnet cake --target="Run-Integration-Tests" --integration-tests-target="Cake.Common.Tools.DotNet.DotNetAliases.DotNetToolInstallListUninstallLocal" --integration-tests-verbosity=Diagnostic --exclusive |
|
There must be something not compatible in my local environment. I tried dotnet cake --target="Run-Integration-Tests"but it failed on Then I tried dotnet cake --target="Run-Integration-Tests" --integration-tests-target="Cake.Common.Tools.DotNet.DotNetAliases.DotNetToolInstallListUninstallLocal" --integration-tests-verbosity=Diagnostic --exclusiveand got Tool GitVersion.Tool is already installed, with required version.
Tool GitReleaseManager.Tool is already installed, with required version.
────────────────────────────────────────────────────────────────────────────────────────────────────
Setup
────────────────────────────────────────────────────────────────────────────────────────────────────
Calculating Semantic Version
Calculated Semantic Version: 6.3.0-implement-dotnet-tool-commands0001 (Version: 6.3.0, Milestone: v6.3.0)
Building version 6.3.0-implement-dotnet-tool-commands0001 of Cake (Release, Run-Integration-Tests) using version 6.2.0.0 of Cake.
(IsTagged: False, IsMainCakeRepo: False, IsMainCakeBranch: False, IsDevelopCakeBranch: False)
(ShouldPublish: False, ShouldPublishToAzureDevOps: False, ShouldSignPackages: False, IsPullRequest: False)
════════════════════════════════════════════════════════════════════════════════════════════════════
Run-Integration-Tests
════════════════════════════════════════════════════════════════════════════════════════════════════
An error occurred when executing task 'Run-Integration-Tests'.
────────────────────────────────────────────────────────────────────────────────────────────────────
TearDown
────────────────────────────────────────────────────────────────────────────────────────────────────
Starting Teardown...
Finished running tasks.
Task Duration Status
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Setup 00:00:01.4025347 Succeeded
Run-Integration-Tests 00:00:00.0110450 Succeeded
Teardown 00:00:00.0017085 Succeeded
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Total: 00:00:01.4152882
Error: Sequence contains no elementsThe same happens with |
|
Cake targets .NET 8, 9, and 10, both unit tests and integration tests are executed on them, you can install .NET SDKs locally just in repo by executing the PowerShell bootstrapper $env:CAKE_INSTALL_SUPPORTED_SDKS='true'
.\build.ps1 --target="Run-Integration-Tests" --integration-tests-target="Cake.Common.Tools.DotNet.DotNetAliases.DotNetTool" --integration-tests-verbosity=Diagnostic |
|
@devlead, because the integration test is registering a local dotnet tool, this should be on an empty directory. What's the best strategy to do that in the integration test? |
paulomorgado
force-pushed
the
feature/implement-dotnet-tool-commands
branch
from
da92706 to
f146fb4
Compare
|
I was able to run the What am I doing wrong? |
paulomorgado
force-pushed
the
feature/implement-dotnet-tool-commands
branch
from
f146fb4 to
5633ef2
Compare
Run-Integration-Tests runs the same integration target three times, once per Cake.dll built (net8.0, net9.0, net10.0). For empty / work folder Paths.Temp + subfolder is the pattern. Which you already seem to partially do, i.e. Shared working directory, DotNetToolExecute and DotNetToolInstallListRunUninstallLocal share ./Cake.Common/Tools/DotNet; when run as part of the full DotNetAliases suite, they are sibling dependencies of DotNetBuildServerShutdown and may interact. Run the execute test in isolation to confirm: dotnet cake --target="Run-Integration-Tests" `
--integration-tests-target="Cake.Common.Tools.DotNet.DotNetAliases.DotNetToolExecute" `
--integration-tests-verbosity=DiagnosticPaths.Temp is cleared as part of the integration tests setup. If you need test isolation, each test should have it's own subfolder I.e. - var workingDirectory = Paths.Temp.Combine("./Cake.Common/Tools/DotNet");
+ var workingDirectory = Paths.Temp.Combine("./Cake.Common/Tools/DotNet/DotNetToolInstallListRunUninstallLocal");Would ensure a dedicated folder for just that test, |
|
@devlead, both |
augustoproiete
changed the title
devlead
force-pushed
the
feature/implement-dotnet-tool-commands
branch
2 times, most recently
from
c1a19d5 to
dc71ec1
Compare
Add typed aliases and settings for dotnet tool subcommands, including install, list, restore, run, search, uninstall, update, and execute. Model command-specific options with dedicated settings classes, keep forwarded tool arguments as explicit ProcessArgumentBuilder parameters for execute/run, and add docs links for each alias. Add unit coverage for command rendering, overloads, settings, forwarded arguments, validation, working directory behavior, and failure handling.
devlead
force-pushed
the
feature/implement-dotnet-tool-commands
branch
from
c4f861c to
bc746f7
Compare
|
@paulomorgado your changes have been merged, thanks for your contribution 👍 |
2 participants
Summary
Adds first-class Cake aliases for the
dotnet toolcommand family, covering the requested subcommands from discussion #4843:DotNetToolExecuteDotNetToolInstallDotNetToolListDotNetToolRestoreDotNetToolRunDotNetToolSearchDotNetToolUninstallDotNetToolUpdateDetails
This introduces command-specific settings types for each supported
dotnet toolsubcommand, matching the existing DotNet alias style in Cake instead of using a generic shared settings/arguments shape.Forwarded tool arguments are only supported where the underlying CLI executes a tool:
DotNetToolExecute(..., ProcessArgumentBuilder arguments, ...)DotNetToolRun(..., ProcessArgumentBuilder arguments, ...)Those forwarded arguments are kept out of the settings classes and are appended after
--, consistent with existing aliases likeDotNetRunandDotNetTest.The aliases also include XML documentation links to the corresponding Microsoft
dotnet toolcommand documentation.Testing
Added unit coverage for: